Skip to content

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Jul 29, 2025

Depends on changes in dotnet/roslyn#79669

@RikkiGibson RikkiGibson requested a review from a team as a code owner July 29, 2025 22:41
@dibarbet

This comment was marked as resolved.

@RikkiGibson

This comment was marked as resolved.

@RikkiGibson RikkiGibson marked this pull request as draft July 30, 2025 00:56
@RikkiGibson
Copy link
Member Author

I gotta say, I am not the most thrilled about certain aspects of javascript debugging. (when the "current statement" being debugged is a whole screenful and then some..)

image

@dibarbet
Copy link
Member

dibarbet commented Aug 6, 2025

I gotta say, I am not the most thrilled about certain aspects of javascript debugging. (when the "current statement" being debugged is a whole screenful and then some..)

Depending on what you are debugging that may be expected or not. If you are debugging the extension code running in the tests, there should be source maps which let you step through the typescript. But if you're debugging VSCode itself, unfortunately thats what we have without a lot more setup.

@RikkiGibson
Copy link
Member Author

@dibarbet pointed out the integration test may be failing because the waiter needs to be explicitly enabled in this context.

The test does pass locally, but, maybe lack of the env var was causing a problem in CI. We will find out.

It looks like it would also be an option to add a command line argument to LanguageServer and call AsynchronousOperationListenerProvider.Enable(true);. But, setting the env var seems a bit easier.

@RikkiGibson
Copy link
Member Author

This is the problem

2025-08-12 19:10:05.471 [debug] [.NET Restore] /__w/1/s/test/lsptoolshost/integrationTests/testAssets/slnWithCsproj/src/scripts/app1.cs(1,1): error MSB4025: The project file could not be loaded. Data at the root level is invalid. Line 1, position 1.
2025-08-12 19:10:05.512 [debug] [.NET Restore] Failed to run restore on /__w/1/s/test/lsptoolshost/integrationTests/testAssets/slnWithCsproj/src/scripts/app1.cs
2025-08-12 19:10:05.513 [debug] [.NET Restore] Restore complete

This probably means an SDK update is needed.


const doRunSuite = process.env.RoslynSkipTestFileBasedPrograms !== 'true';
console.log(`process.env.RoslynSkipTestFileBasedPrograms: ${process.env.RoslynSkipTestFileBasedPrograms}`);
const doRunSuite = process.env['ROSLYN_SKIP_TEST_FILE_BASED_PROGRAMS'] !== 'true';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://learn.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch#environment-variables

System and user-defined variables (except secret variables) also get injected as environment variables for your platform. When variables convert into environment variables, variable names become uppercase, and periods turn into underscores. For example, the variable name any.variable becomes the variable name $ANY_VARIABLE.

Linux env vars are case sensitive, so, the env. lookup is case sensitive. Since we were not using an uppercase name to access the env var, we weren't seeing it in the test.

Figuring this out took an unreasonable amount of time.

@RikkiGibson
Copy link
Member Author

Mac failure:

FAIL test/lsptoolshost/integrationTests/fileBasedPrograms.integration.test.ts (28.106 s)
  ● File-based Programs Tests › Inserting package directive triggers a restore

    Polling did not succeed within the alotted duration: Error: expect(received).toContain(expected) // indexOf

    Expected value: "Newtonsoft"
[main 2025-10-29T01:31:55.708Z] Extension host with pid 15201 exited with code: 1, signal: unknown.
    Received array: ["Microsoft", "static", "System", "unsafe"]

      270 |     }
      271 |
    > 272 |     throw new Error(`Polling did not succeed within the alotted duration: ${error}`);
          |           ^
      273 | }
      274 |
      275 | export async function sleep(ms = 0) {

      at waitForExpectedResult (integrationHelpers.ts:272:11)
      at Object.<anonymous> (fileBasedPrograms.integration.test.ts:55:9)

Maybe the remaining failing legs are using too old of an SDK? They may need to also have the env var added.

@RikkiGibson RikkiGibson marked this pull request as ready for review October 30, 2025 17:20
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 2, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 2, 2025
@RikkiGibson
Copy link
Member Author

@JoeRobich @dibarbet This is ready for review


const doRunSuite = process.env['ROSLYN_SKIP_TEST_FILE_BASED_PROGRAMS'] !== 'true';
console.log(`process.env.ROSLYN_SKIP_TEST_FILE_BASED_PROGRAMS: ${process.env.ROSLYN_SKIP_TEST_FILE_BASED_PROGRAMS}`);
console.log(`doRunSuite: ${doRunSuite}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider extracting all this to a helper function similar to

export const describeIfCSharp = describeIf(!usingDevKit());

let exports: CSharpExtensionExports;

beforeAll(async () => {
process.env.RoslynWaiterEnabled = 'true';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking it would be reasonable to set this for all Roslyn integration test runs, even if we don't use it yet.
e.g.

process.env.RUNNING_INTEGRATION_TESTS = 'true';

I can look at that in a followup though myself.

displayName: Test Windows
dependsOn: []
variables:
ROSLYN_SKIP_TEST_FILE_BASED_PROGRAMS: 'true'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can enable the tests for windows/mac when we upgrade them to running on .NET 10?

@@ -0,0 +1,4 @@

using Newton;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having this be in the file by default, should we insert it during the test? That way we can re-use the same base test file if we need to for other tests?

@RikkiGibson RikkiGibson enabled auto-merge (squash) November 4, 2025 21:24
@RikkiGibson RikkiGibson merged commit 01dd584 into main Nov 4, 2025
33 checks passed
@RikkiGibson RikkiGibson deleted the dev/rigibson/integration-test-restore branch November 4, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants